Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

winit 0.29: Ergonomics, examples + keymap cache #3

Closed
wants to merge 44 commits into from

Conversation

Vrixyz
Copy link
Owner

@Vrixyz Vrixyz commented Jun 16, 2023

Context for this PR: bevyengine#8745 .

Dynamic key mapping to rely on physical key rather than only logical key

Problem description

winit 0.29 logical key with modifiers (shift) is hard to use cache with, because winit can send pressed('A'), and then released('a').

So Res::<Key>::is_pressed('A') returns true when pressing 'A' then releasing 'a'. That's not correct!

Details

A branch without the Dynamic key mapping can be tried there: https://github.com/Vrixyz/bevy/tree/update-winit-without-dynamic-key-mapping

Detailed scenario

Bevy adds a helper function (is_pressed) to know if a key is currently pressed or not, we react to winit::event::KeyboardInput and cache its value when changed.

Now, I adapted the code to the more customizable "SmolStr" ; and I'm having issues when caching this value with modifiers, because the logical key changes to a "released" without being pressed

We receive: (azerty keyboard)
1: press shift right

KeyEvent {
    physical_key: ShiftRight,
    logical_key: Shift,
    text: None,
    location: Right,
    state: Pressed,
    repeat: false,
    platform_specific: KeyEventExtra {
        text_with_all_modifers: None,
        key_without_modifiers: Shift,
    },
}

2: press "a" (shift right still pressed)

KeyEvent {
    physical_key: KeyQ,
    logical_key: Character(
        "A", // <--- We store cache["A"] = pressed
    ),
    text: Some(
        "A",
    ),
    location: Standard,
    state: Pressed,
    repeat: false,
    platform_specific: KeyEventExtra {
        text_with_all_modifers: Some(
            "A",
        ),
        key_without_modifiers: Character(
            "a",
        ),
    },
}

3: release Shift Right, "a" still pressed

KeyEvent {
    physical_key: ShiftRight,
    logical_key: Shift,
    text: None,
    location: Right,
    state: Released,
    repeat: false,
    platform_specific: KeyEventExtra {
        text_with_all_modifers: None,
        key_without_modifiers: Shift,
    },
}

4: release "a"

KeyEvent {
    physical_key: KeyQ,
    logical_key: Character(
        "a", // <--- We store cache["a"] = released;
    ),
    text: None,
    location: Standard,
    state: Released,
    repeat: false,
    platform_specific: KeyEventExtra {
        text_with_all_modifers: None,
        key_without_modifiers: Character(
            "a",
        ),
    },
}

Detailed solution

Problem recap

With shift modifier:

user sends keypress(logical = "A" ; KeyCode::KeyQ)

user might release key: (logical = "a" ; KeyCode::KeyQ)

So is_pressed("A") cannot rely on is_released("A")

My solution

When a bevy user asks is_pressed/released("A") -> bevy should check on the more reliable KeyCode::KeyQ.

To map both together, I store the physical key (KeyCode), alongside a map between logical key and physical key.

  • press("A", KeyCode::Q)) -> adds a KeyCode::Q, + a mapping logic("A") -> KeyCode::Q
  • is_pressed("A") -> check if we have a mapping with logic("A"), we have, so we check our stored KeyCode::Q
  • release("a") -> we use the reliable KeyCode::Q from winit to release logical key.

New aggregated resource Res<Input<KeyLogic>>

This is less necessary, but I noticed a lot of duplicated code between Res<Input<KeyCode>> and Res<Input<Key>>, this is an attempt to simplify the API and improve maintainability.

Resources impacts

Breaking: removed Res<Input<KeyCode>> and Res<Input<ScanCode>>, in favor of Res<Input<KeyLogic>>

  • ScanCode information can be inferred within KeyLogic::PhysicalKey.
  • a new concept of LogicalKey (depending on OS software layout) can be accessed through KeyLogic::Key.

API impacts

These key snippets assume a keys: Res<Input<KeyLogic>>

You can now query for physical or logical key codes with:

  • Physical: keys.pressed(KeyLogic::Physical(KeyCode::KeyA))
  • Logical: keys.pressed(KeyLogic::Logic(Key::Character(SmollStr::new("a")))

Through From trait impls, ergonomics are still good:

  • Physical: keys.pressed(KeyCode::KeyA)
  • Logical: keys.pressed("a")
    • or keys.pressed(KeyLogic::Logic("a"))
    • or keys.pressed(Key::Character("a"))

@Vrixyz Vrixyz force-pushed the update-winit-dynamic-key-mapping branch 2 times, most recently from c8653c4 to 92a98f0 Compare June 16, 2023 08:16
@Vrixyz Vrixyz force-pushed the update-winit-dynamic-key-mapping branch from 92a98f0 to ea9339c Compare June 16, 2023 09:12
Comment on lines +54 to +57
dynamic_map_value: HashMap<T, T>,
}

impl<T: Clone + Eq + Hash + Send + Sync + 'static> Default for Input<T> {
impl<T: FromReflect + Clone + Eq + Hash + Send + Sync + 'static> Default for Input<T> {
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To allow FromReflect on Input<T>, T needs to be FromReflect too because of the HashMap, it's a bit unfortunate because it adds a constraint on T.

@Vrixyz Vrixyz force-pushed the update-winit-dynamic-key-mapping branch from f239452 to 3e4c560 Compare July 20, 2023 15:36
@Vrixyz Vrixyz changed the title dynamic key mapping to rely on physical key rather than logical key dynamic key mapping to rely on enum key rather than logical key Aug 28, 2023
@Vrixyz Vrixyz changed the title dynamic key mapping to rely on enum key rather than logical key dynamic key mapping to rely on enum key rather than only logical key Aug 28, 2023
@Vrixyz Vrixyz changed the title dynamic key mapping to rely on enum key rather than only logical key dynamic key mapping to rely on physical key rather than only logical key Aug 28, 2023
Vrixyz added a commit that referenced this pull request Oct 25, 2023
Vrixyz added a commit that referenced this pull request Oct 25, 2023
Vrixyz added a commit that referenced this pull request Oct 25, 2023
pub enum KeyLogic {
/// Represents the logical key, typically what's visible on the keyboard.
Logic(Key),
/// Represents the ScanCode

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a confusing comment.

derive(serde::Serialize, serde::Deserialize),
reflect(Serialize, Deserialize)
)]
pub enum KeyLogic {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This name isn't good: it's hard to intuit what it might mean and it's quite unnatural in English.

/// A collection of every button that is currently being pressed.
pressed: HashSet<T>,
/// A collection of every button that has just been pressed.
just_pressed: HashSet<T>,
/// A collection of every button that has just been released.
just_released: HashSet<T>,
/// To map logical keys with their respecting physical keys for reliability.
dynamic_map_value: HashMap<T, T>,
Copy link

@alice-i-cecile alice-i-cecile Nov 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a very interesting change. I'm not sure I agree with the design: why would the physical and logical keys be the same type?

Copy link
Owner Author

@Vrixyz Vrixyz Nov 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is led by the inability from winit to be sure a key_pressed(Character("a")) leads to a key_released(Character("a")), so bevyengine#8745 believes character("a") is still pressed.

To understand the issue, I invite you to try the example keyboard_input.

I have to keep track on original keycode to update its cached keypress status.

Now I could have made that change only for Input (logical key), but as the data model is shared, that would have led to code duplication, which I thought was not worth it.

Another option would be to open an issue on winit, to evaluate if we can have a character released without the modifiers, or an event for key_change: from a modifier (lowercase) to another (uppercase) 🤔

derive(serde::Serialize, serde::Deserialize),
reflect(Serialize, Deserialize)
)]
pub enum KeyLogic {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unifying these into a single type seems incorrect: it seems likely to increase confusion and boilerplate. See comment about about the HashMap.

@alice-i-cecile
Copy link

Very interesting set of changes, but I really think we should leave this out of the 0.29 PR. That one is hard to understand and review as is, and this set of changes deserves a full round of review in its own right.

@Vrixyz
Copy link
Owner Author

Vrixyz commented Nov 17, 2023

@alice-i-cecile I'd love to isolate this PR out, but then how do I fix #3 (comment) ?

Another option might be to not handle Character(SmolStr) in Input: only support it in the ReceivedCharacters events 🤔

@Vrixyz
Copy link
Owner Author

Vrixyz commented Dec 21, 2023

that's abandoned in favor of exposing raw KeyEvent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants